-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add proposal for "ref and unsafe in iterators and async" #7994
Conversation
17d0312
to
6053571
Compare
a260c81
to
9a2308f
Compare
[§13.3.1 Blocks > General][blocks-general]: | ||
|
||
> ~~It is a compile-time error for an iterator block to contain an unsafe context ([§23.2][unsafe-contexts]).~~ | ||
> An iterator block always defines a safe context, even when its declaration is nested in an unsafe context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use ```diff
blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's just me, but for text/spec changes strikethrough renders more nicely (because other text formatting is still applied). Diff blocks are rendered as plain text, which doesn't seem great for this purpose.
|
||
> [...] | ||
> | ||
> **A warning is reported (as part of the next warning wave) when a `yield` statement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only for legacy lock
? I assume it will be an error to yield out of a "modern" lock. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the modern lock cannot work with yield
since it's lowered to using
of a ref struct
. I will make it clearer.
> | ||
> **It is a compile-time error for an unsafe context ([§23.2][unsafe-contexts]) to contain `await` or `yield`.** | ||
|
||
Furthermore, variables inside async or iterator methods should not be "fixed" but rather "moveable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change? Is this going to require a user to use fixed
depending on whether the local is hoisted or not? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is a breaking change. No, the intention is to disallow taking pointers of such locals entirely - that's consistent with how variables captured in lambdas and local functions work today. Although allowing to use fixed
with those variables (both captured and hoisted) is a possible alternative or future work. I guess there are also other options:
- Require
fixed
for all locals in async methods (no need to distinguish hoisted or not). - Leave it as is, maybe emit only a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discovered there is already a warning for this in C# 12 warning wave (which is likely why it's not visible in sharplab): dotnet/roslyn#66915
Given that we already decided to go with a warning here, I will remove this part of the feature, only leaving a note in alternatives.
ref int x = ...; | ||
x.ToString(); | ||
await Task.Yield(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a class of ref struct
errors that are demoted to warnings inside an unsafe
context. Think we should call out explicitly that this is not one of them. Essentially that this will remain and error even in an unsafe
context.
These type of values cannot be safely manipulated in unsafe
without relying on implementation details of how the state machine rewrite works. That falls outside the boundaries of what we've allowed to demote to warning in unsafe
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safely manipulated in
unsafe
:D
|
||
- Allow `ref`/`ref struct` locals and `unsafe` blocks in iterators and async methods | ||
provided they are used in code segments without any `yield` or `await`. | ||
- Warn about `yield` inside `lock`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of saying the specific items we'll fix consider having a more general "unify behavior between iterators and async methods" statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add that summarizing sentence, that's a good idea, but I will still leave the specific items as well because it's otherwise hard to tell at glance which specific things change.
x.ToString(); | ||
await ...; | ||
// x.ToString(); // still error | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following code:
async Task<int> M()
{
int x = 42;
ref int y = ref x;
y.ToString();
await Task.Delay();
y = ref x; // error or okay?
y.ToString(); // error or okay?
In this case we're reassigning the ref
local so it doesn't need to be hoisted. Is this allowed or not?
I would lean towards "do not allow" until a user could provide fairly compelling reasons why it should be. But this part of the proposal is leaning on allowing behavior when the ref
doesn't need to be hoisted hence I think we should call out specifically this case in the spec. Essentially when we re-assign after await / yield
is that legal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any specific concerns why we should disallow this? We could certainly disallow ref reassignments after await/yield and potentially lift the restriction later, but we are already lifting restrictions here, so I would lean towards allowing this.
Note that this would be allowed by the most straightforward implementation (removing some binding errors but leaving the after-lowering walker which finds usages of ref-like locals across awaits/yields).
Also your example seems equivalent to the following, so we would presumably need to disallow that as well:
int x = 42;
Span<int> y = new(ref x);
y.ToString();
await Task.Yield();
y = new(ref x);
y.ToString();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any specific concerns why we should disallow this?
My concerns are complications of implementation. If this is straight forward to do then I'd be more okay with going this direction. Consider though that there seem to be real challenges with doing this.
// This will be legal in C# 13
ref struct S : IEnumerable<int> { .. }
async Task M(S s)
{
foreach (var i in s) {
await Task.Delay();
}
}
The value s
here is definitely used after the await
but the current spec language seemingly allows it. This same problem comes up when a ref struct
is used in any block level construct: foreach
, using
, lock
, etc ... Is the expectation that for all of these types of constructs that if they involve a ref struct
that we error when a await / yield
occurs inside them?
Also I'm wondering about the mechanism by which this feature is implemented. The spec says it's an error to use it. Did you consider as an alternative using definite assignment as a mechanism? One approach I was thinking of is essentially the following:
After a statement with a
await
oryield
allref struct
orref
locals in scope are considered definitely unassigned.
That would seemingly provide all the behavior that we want but I haven't really dug into the implementation to see if it's practical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value
s
here is definitely used after theawait
but the current spec language seemingly allows it.
It was my intention to disallow this but the spec language should be clarified, you're right. Thanks.
Is the expectation that for all of these types of constructs that if they involve a
ref struct
that we error when aawait / yield
occurs inside them?
Yes.
Also I'm wondering about the mechanism by which this feature is implemented. The spec says it's an error to use it. Did you consider as an alternative using definite assignment as a mechanism?
That's exactly what I want to do. There is already a definite assignment walker (IteratorAndAsyncCaptureWalker
) which does what you suggest. It only supports ref struct
s now, so needs some small tweaking to support ref
locals as well. Plus I will remove all the binding errors that restrict async
with ref
/ref struct
locals, etc. And that's it. Note that the walker runs after lowering, so it automatically handles foreach
/using
/lock
etc.
I'm not sure if the "definite assignment" mechanism should be part of the spec language as well. But I will try to incorporate it to clarify things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 14)
No change in the spec is needed to allow `unsafe` blocks which do not contain `await`s in async methods, | ||
because the spec has never disallowed `unsafe` blocks in async methods. | ||
However, the spec should have always disallowed `await` inside `unsafe` blocks | ||
(it had already disallowed `yield` in `unsafe` in [§13.3.1][blocks-general] as cited above), for example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jaredpar @AlekseyTs
Rendered markdown